Changed sharedBlacklist RegExp due to a bug with Node 12.11.0 #464
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NodeJs updated in version 12.11.0 to the V8 engine 7.7.7.299.11 (i think the update to the newer V8 is the reason why this is broken) since then the sharedBlacklist regexp was broken due to the function sharedBlacklist() witch in a earlier version from NodeJs returned for the first element in sharedBlacklist this `/node_modules[\\\\]react[\\\\]dist[\\\\].*/` output. In the newer version of NodeJs the output is different `/node_modules[\\\]react[\\\]dist[\\\].*/` witch is not valid anymore and it trows a error. With my little change `/node_modules[\/\\]react[\/\\]dist[\/\\].*/` it works again on both versions Output NodeJs version 12.11.0: `/node_modules[\\\\]react[\\\\]dist[\\\\].*/` Output NodeJs version 10.15.2: `/node_modules[\\\\]react[\\\\]dist[\\\\].*/`
facebook-github-bot
added
the
CLA Signed
Codecov Report
@@ Coverage Diff @@
## master #464 +/- ##
=======================================
Coverage 84.43% 84.43%
=======================================
Files 173 173
Lines 5764 5764
Branches 949 949
=======================================
Hits 4867 4867
Misses 794 794
Partials 103 103
Continue to review full report at Codecov.
|
This was referenced Oct 20, 2019
|
Thank you for your PR! Fixed in #468. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Oct 29, 2019
Summary: **Summary** On Windows with Node.js v12.x.x, Metro crashes with ``` SyntaxError: Invalid regular expression: /(.*\\__fixtures__\\.*|node_modules[\\\]react[\\\]dist[\\\].*|website\\node_modules\\.*|heapCapture\\bundle\.js|.*\\__tests__\\.*)$/: Unterminated character class ``` This has been reported in #453, facebook/react-native#26829, facebook/react-native#26969, facebook/react-native#26878, facebook/react-native#26598, expo/expo-cli#1147 and expo/expo-cli#1074. There are a few open pull requests attempting to fix this same issue: * #464 * #461 * #458 * #454 However, none of the existing PRs address the *root cause* of this error: the `escapeRegExp` function in `blacklist.js` tries to convert regular expressions to be agnostic to the path separator ("/" or "\\"), but turns some valid regular expressions to invalid syntax. The error was is this line: https://github.com/facebook/metro/blob/142348f5345e40ce2075fc7f9dfa30c5d31fee2a/packages/metro-config/src/defaults/blacklist.js#L28 When given a regular expression, such as `/node_modules[/\\]react[/\\]dist[/\\].*/`, on Windows where `path.sep` is `\` (which is also an escape character in regular expressions), this gets turned into `/node_modules[\\\]react[\\\]dist[\\\].*/`, resulting in the `Unterminated character class` error. Automatically replacing `[/]` with `[\]` is an error, as is replacing `[\/]` with `[\\]`, because in both of these cases the backslash before the end of character class "]" escapes it, and the character class becomes unterminated. Therefore, this PR changes the code to look for both escaped forward slash `\/` and forward slash `/`, and always replace them with the escaped version (`\/` or `\\`, depending on the platform). This fixes #453. **Test plan** Added a test case that exercises the code with both `\` and `/` as path separators. Pull Request resolved: #468 Differential Revision: D18201730 Pulled By: cpojer fbshipit-source-id: 6bb694178314c39d4d6a0fd9f8547bfa2c36f894
|
It fixed! |
cpojer
pushed a commit
that referenced
this pull request
Jan 8, 2020
Summary: **Summary** On Windows with Node.js v12.x.x, Metro crashes with ``` SyntaxError: Invalid regular expression: /(.*\\__fixtures__\\.*|node_modules[\\\]react[\\\]dist[\\\].*|website\\node_modules\\.*|heapCapture\\bundle\.js|.*\\__tests__\\.*)$/: Unterminated character class ``` This has been reported in #453, facebook/react-native#26829, facebook/react-native#26969, facebook/react-native#26878, facebook/react-native#26598, expo/expo-cli#1147 and expo/expo-cli#1074. There are a few open pull requests attempting to fix this same issue: * #464 * #461 * #458 * #454 However, none of the existing PRs address the *root cause* of this error: the `escapeRegExp` function in `blacklist.js` tries to convert regular expressions to be agnostic to the path separator ("/" or "\\"), but turns some valid regular expressions to invalid syntax. The error was is this line: https://github.com/facebook/metro/blob/142348f5345e40ce2075fc7f9dfa30c5d31fee2a/packages/metro-config/src/defaults/blacklist.js#L28 When given a regular expression, such as `/node_modules[/\\]react[/\\]dist[/\\].*/`, on Windows where `path.sep` is `\` (which is also an escape character in regular expressions), this gets turned into `/node_modules[\\\]react[\\\]dist[\\\].*/`, resulting in the `Unterminated character class` error. Automatically replacing `[/]` with `[\]` is an error, as is replacing `[\/]` with `[\\]`, because in both of these cases the backslash before the end of character class "]" escapes it, and the character class becomes unterminated. Therefore, this PR changes the code to look for both escaped forward slash `\/` and forward slash `/`, and always replace them with the escaped version (`\/` or `\\`, depending on the platform). This fixes #453. **Test plan** Added a test case that exercises the code with both `\` and `/` as path separators. Pull Request resolved: #468 Differential Revision: D18201730 Pulled By: cpojer fbshipit-source-id: 6bb694178314c39d4d6a0fd9f8547bfa2c36f894
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NodeJs updated in version 12.11.0 to the V8 engine 7.7.7.299.11 (i think the update to the newer V8 is the reason why this is broken) since then the sharedBlacklist regexp was broken due to the function sharedBlacklist() witch in a earlier version from NodeJs returned for the first element in sharedBlacklist this
/node_modules[\\\\]react[\\\\]dist[\\\\].*/output.In the newer version of NodeJs the output is different
/node_modules[\\\]react[\\\]dist[\\\].*/witch is not valid anymore and it trows a error.With my little change
/node_modules[\/\\]react[\/\\]dist[\/\\].*/it works again on both versionsOutput NodeJs version 12.11.0:
/node_modules[\\\\]react[\\\\]dist[\\\\].*/Output NodeJs version 10.15.2:
/node_modules[\\\\]react[\\\\]dist[\\\\].*/Summary
In the React-Native repo there are many Issues regarding this bug
a few examples
facebook/react-native#26829
facebook/react-native#26598
facebook/react-native#26808
With this small change all the Issues should be fixed
Test plan
Without my Change:
Output NodeJs version 12.11.0:
/node_modules[\\\]react[\\\\]dist[\\\].*/Output NodeJs version 10.15.2:
/node_modules[\\\\]react[\\\\]dist[\\\\].*/With my Change:
Output NodeJs version 12.11.0:
/node_modules[\\\\]react[\\\\]dist[\\\\].*/Output NodeJs version 10.15.2:
/node_modules[\\\\]react[\\\\]dist[\\\\].*/i did not wrote any tests
Other
this is my first pull request for a Facebook product and I've accepted the Facebook CLA